Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[R4R] fix: memory leak issue with diff protocol #1019

Merged
merged 1 commit into from
Jul 26, 2022
Merged

[R4R] fix: memory leak issue with diff protocol #1019

merged 1 commit into from
Jul 26, 2022

Conversation

j75689
Copy link
Contributor

@j75689 j75689 commented Jul 26, 2022

Description

broadcastDiffLayers is not properly closed, the memory accumulates too many resources and causes OOM.

goroutine 8845520 [select, 3426 minutes]:
github.com/ethereum/go-ethereum/eth/protocols/diff.(*Peer).broadcastDiffLayers(0xc081628d20)
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:47 +0xab
created by github.com/ethereum/go-ethereum/eth/protocols/diff.NewPeer
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:41 +0x2f0

goroutine 7119763 [select, 3531 minutes]:
github.com/ethereum/go-ethereum/eth/protocols/diff.(*Peer).broadcastDiffLayers(0xc10bcd4e40)
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:47 +0xab
created by github.com/ethereum/go-ethereum/eth/protocols/diff.NewPeer
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:41 +0x2f0

goroutine 11443031 [select, 3272 minutes]:
github.com/ethereum/go-ethereum/eth/protocols/diff.(*Peer).broadcastDiffLayers(0xc0f9bb3260)
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:47 +0xab
created by github.com/ethereum/go-ethereum/eth/protocols/diff.NewPeer
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:41 +0x2f0

Rationale

broadcastDiffLayers will be started in NewPeer; need to call peer.Close when RunPeer fails. otherwise, this goroutine will not be closed.

Example

panic.log

Changes

Notable changes:

  • close the peer on failure in RunPeer function.

brilliant-lx
brilliant-lx approved these changes Jul 26, 2022
View reviewed changes
Copy link
Collaborator

@brilliant-lx brilliant-lx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@unclezoro unclezoro changed the title [R4R] memory leak issue with diff protocol [R4R] fix: memory leak issue with diff protocol Jul 26, 2022
@unclezoro unclezoro merged commit dd3b3a6 into bnb-chain:develop Jul 26, 2022
@forcodedancing
Copy link
Contributor

LGTM

This was referenced Jul 28, 2022
Merged
Merged
@0xe3b0c4 0xe3b0c4 mentioned this pull request Aug 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brilliant-lx brilliant-lx brilliant-lx approved these changes

@forcodedancing forcodedancing forcodedancing approved these changes

@unclezoro unclezoro unclezoro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@j75689 @forcodedancing @unclezoro @brilliant-lx